Skip to content

feat: add the connected-app consent screen with step-up, and run the OAuth server in Production - #1144

Merged
mforce merged 18 commits into
mainfrom
feat/798-oauth-consent
Oct 9, 2026
Merged

mforce merged 18 commits into
mainfrom
feat/798-oauth-consent

Conversation

@mforce

@mforce mforce commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Closes #798

Slice 4 of #788. This PR replaces #795's self-approving stub with real consent, adds the consent screen a connected app sends people to, and turns the OAuth server on in Production.

Before a release with this change is deployed

The deployment repo must set OAuth__Issuer to the public https URL, for example https://farm.example/. Without it the Production serving container refuses to start. One-shot verbs (migrate, bootstrap-admin and the others) do not need it.

Release note

This PR must ship in the same release as #799 (Connected apps on Account). The consent screen tells people to "Disconnect it anytime in Account › Connected apps", and #799 builds that place. The maintainer's rule is that no release is cut between merging #798 and #799.

What people see

Screenshots at 1280 and 390, from a stack rebuilt at the head, are in the screenshot comment on this PR. The picks are in docs/designs/788-oauth-screens/SELECTION.md, and the direction lab is beside it.

  • Login B. When sign-in continues to a connected app, the brand panel says "Next: approve ". On a phone, the box sits above the form. The app name comes from an anonymous preview of the same validated authorize request, and it returns nothing but the app's self-chosen name.
  • Consent D, at /connect. A compact card: the question, an Unverified app marker, two permission rows, where the browser returns, and "Acts as you, with only your access" with the user's role. Then Your current password, Allow and Cancel, the undo line, and a closed Details section. The signed-in email sits at the top in normal text colour and semibold weight, and Not you? Sign out is in Details. Details states what the request would reach: its scopes, within the user's role and flock scope. A Worker with assignments reads the assigned flocks' names; a Worker with none, like every other role, reads every flock. Focus starts at the headline, so a screen reader begins at the request and a phone keeps its keyboard closed; a wrong password returns focus to the field. "Record daily entries" makes no promise about drafts; MCP slice 6: write tool — record daily entry #809 adds that line once the write tool's behaviour is settled. A role that cannot record entries sees "Not allowed for your role" on that row.
  • Asks for more. The new permission comes first, tinted and marked New; the earlier one reads Already allowed.
  • Reconnect. An app already approved for everything it asks gets a slim card that asks for the password only.
  • One step-up field. StepUpPasswordField replaces the six inline copies on the Users screen and serves the consent card. The 167 Users specs pass unchanged.
  • Copy is in en, es and tl, with Help (a "Connecting an app" bullet) and the in-app glossary and GLOSSARY.md ("Connecting an app", next to OAuth slice 6: audit provenance — record which application acted, alongside the human #800's "Connected app").

Precache

The maintainer approved a second milestone raise, to 1,950 KiB. web/scripts/verify-sw.mjs and docs/designs/822-mui-revamp.md record it beside #800's earlier note. After merging main with #800, this branch measures 1,923.35 KiB, so #798 adds about 20 KiB on top of #800. About 10 KiB of that is strings in three languages, and the rest is code. The planned way to win the space back is to stop precaching the languages a device does not use, which is tracked separately.

How the flow works

The SPA holds its access token in tab memory. A browser navigation to the authorize endpoint therefore carries no bearer, and fetch cannot read a cross-origin redirect. So GET /api/v1/oauth/authorize answers three callers.

Caller Answer
A browser navigation, with no bearer A 302 to /connect with the same query, which OpenIddict has already validated
The SPA, with its session bearer 200 with what the consent screen shows, or 200 { redirectUri } for the SPA to navigate to
The SPA, with an expired bearer 401, so the SPA refreshes and asks again

Allow repeats the request with X-Cluckwork-Step-Up. Cancel repeats it with X-Cluckwork-Consent: deny, and the client then receives access_denied. The consent payload is { clientId, clientName, redirectHost, scopes, alreadyAllowed, alreadyApproved, assignedFlocks }. assignedFlocks is the flock scope this request resolved: null for every flock, else the assigned flocks' names, read through IAccessModule. When alreadyApproved is true, the SPA asks for the password only.

Decisions

  • The new IAccessModule method. ConsumeStepUpGrantAsync(accountId, userId, stepUpToken, ct) validates and spends a Require recent step-up authentication for privileged user administration #308 grant, the same way the user-administration handlers do. IStepUpGrantService stays internal to Access. The ledger gains the adapter row OAuthEndpoints.Authorize -> Access.
  • Consent rules. All or nothing, and no path issues a code without a spent step-up grant. Approval creates a permanent authorization that holds the requested scopes. If one valid permanent authorization of the same user for the same app already holds every requested scope, the payload sets alreadyApproved, and the reconnect asks for the password only and reuses that authorization. A wider request shows the permissions again, and alreadyAllowed names what was approved before.
  • The redirect host shown on consent comes from the redirect URI that OpenIddict validated. A client with one registered URI may omit redirect_uri.
  • Scopes. The two scopes are farm:read and daily-entries:write. A request with no scope defaults to farm:read. Scope permissions are ignored per client, because every client registers itself anonymously.
  • The Production config key. A Production serving process requires OAuth:Issuer. It must be an absolute https URL with no query or fragment, and the check is serving-only (refactor(api): make process role explicit for boot guards #347). The sim harness, deploy/.env.example, ProcessRoleGuardTests (two rows) and ServingGuardCoverageTests all carry it. The AppHost runs Development, where appsettings.Development.json already sets an issuer. Its tests pass unchanged.
  • Discovery URLs come from the issuer. OAuth slice 1: stand up OpenIddict — tables, migration, token issuance #795's discovery used whatever Host the client called. Behind a proxy that Host can be an internal name, so discovery now rebases the authorization, token and registration endpoints onto the issuer.
  • The return path after sign-in. Login reads its destination from router state, never from a query parameter. returnPath sends anything that resolves to another origin back home. That covers absolute URLs, //host and /\host. Only /connect keeps its query.

Decision: a reconnect skips the permissions, never the password

The first version skipped both the screen and the password when an app asked for nothing new. Review showed what that allowed. Someone with only a stolen session access token could get a code for an already-approved app from the JSON response. That code becomes a token that never expires. The maintainer decided that every connection asks for the password, and a reconnect skips only the permission explanation. App tokens never expire, so people reconnect rarely, and the password costs little each time. docs/decisions/798-oauth-consent.md records the decision and the alternative it rejected, which was validating the refresh cookie as a second browser proof.

Proof

  • OAuthConsentTests (new) drives each consent rule. A bearer-only reconnect gets no code. A reconnect spends its grant, and a replayed grant is refused. The suite also covers the JSON redirect with state and iss, cancel, a signed session JWT expired past the 30-second skew, a malformed bearer, an omitted redirect_uri, the read-only default, refused response modes, an unknown scope, and discovery from a second host.
  • OAuthConsentTests.Preview_WithoutASession_NamesTheAppOnly pins the login preview to the app's name alone.
  • OAuthServerProductionTests runs the whole connect flow in a Production host: registration, the navigation hand-off, consent, step-up and token redemption. It also shows that four bad issuers stop the host from starting, a bare # among them.
  • Local runs: the OAuth filter (102/102) and CredentialEpochTests; ProcessRoleGuardTests (58/58), ServingGuardCoverageTests, FixturePortRegistrationTests and OtlpSubprocessExporterTests (11/11) earlier in this PR; the Application architecture tests; the AppHost tests; tsc; and the Vitest specs below.
  • Vitest: 18 ConnectPage specs. They cover each screen state (normal, wrong password, asks for more, reconnect, Details and sign-out, a role that cannot record, a web app's host, cancel, refused, the prominent email). They also cover Details for a Worker with assignments, a Worker with none, a Manager asking only to read, and a Manager asking for both, plus initial focus for consent and reconnect and focus after a wrong password. Also 3 Login B specs, 1 AuthShell spec, the 167 Users specs on the shared field, and the style, i18n, Help and glossary guards.
  • Vitest mutations on the consent page. Ignoring assignedFlocks fails the scoped-Worker spec, and choosing the write sentence by role alone fails the read-only spec. Removing the heading focus fails both initial-focus specs, and removing the retry focus fails the wrong-password spec. Putting autoFocus back on the password survives, as an equivalent mutant: the heading-focus effect runs after the input mounts and takes focus back, so focus still starts at the request.
  • tools/oauth/mutation-check.sh has 86 mutants. The full run at 6368134 killed 83 and held redirect-fragment, where OpenIddict still refuses the URI. After the merge with OAuth slice 6: audit provenance — record which application acted, alongside the human #800 and review round 3, I reran all 27 mutants on OAuthEndpoints.cs, which both changed, at 887a7b2. They include the two new ones: scoped-worker-unrestricted (a scoped Worker told every flock) and unassigned-worker-scoped (an unassigned Worker told a flock list). All 27 are killed, with baseline and restore green at 105/105. One of them, reconnect-without-password, needed its anchor regenerated for the payload's new field first. The 59 mutants on other files last ran at 6368134; the merge changed none of the files they mutate.
  • Vitest mutations. Removing the origin check fails the absolute and protocol-relative cases. Dropping the /connect query fails the Login test.
  • tools/simulation/verify-harness.sh passes the bootstrapped .env.sim and refuses OAuth__Issuer=https://cluckwork-sim.local/?tenant=1, and it now refuses a bare ? or # as the boot guard does.
  • Rate-limit flakes, fixed. The fixed window is aligned to the clock, so two requests that straddle a 60-second boundary both pass. AuthorizeEndpoint_IsRateLimited failed that way in CI, and TokenEndpoint_IsRateLimited failed that way locally. Both tests came from OAuth slice 2: fail-closed checks on the OAuth path (disabled, suspended, must-change-password, flock scope, rate limit) #796. Commit 2624649 pins a one-day window in the tests that tighten an OAuth limit, as CI: root-cause the four remaining integration flakes #840 did for MultiInstanceRateLimitTests.
  • Service worker swallows server-issued redirects, so ?farm= never reaches the login form #778, on a built SPA with the service worker active. The isolated cw798 stack ran Production config, rebuilt at the head, behind an https edge with a matching OAuth__Issuer. A browser navigation to the authorize endpoint went to /connect and then to /login, which named the app, with the service worker in control. After sign-in, the browser returned to /connect with the request intact. Allow with the password reached the app's loopback callback with code and state, and a password-only reconnect issued a code that redeemed with 200. One earlier run of the capture script timed out waiting for the callback on the last reconnect. The app logged no error for it, and three reruns passed, so it is recorded here rather than explained.

For #799 and later

  • Disconnect must revoke every valid authorization of the user for that app. Two approvals that race can create two.
  • prompt and max_age are ignored, so prompt=consent cannot force the screen.
  • Whether connecting and cancelling should write audit events is still open (question 4 on the mockups).

mforce added 3 commits October 9, 2026 04:43
…th server in Production

Replaces #795's self-approving stub. A browser navigation to the authorize
endpoint carries no session bearer, so it is handed to the SPA's /connect
route with its query. The SPA asks the same endpoint with its bearer and
gets either what the consent screen shows or the client's redirect as JSON,
because fetch cannot read a cross-origin redirect.

Approval spends a step-up grant through the new
IAccessModule.ConsumeStepUpGrantAsync and records a permanent authorization.
A later request that asks for nothing new is approved silently; one that asks
for more is shown again. The two scopes are farm:read and
daily-entries:write.

A Production serving process now runs the authorization server and refuses
to start without an absolute https OAuth:Issuer. Discovery names its
endpoints under the issuer rather than the request Host. The sim harness and
deploy/.env.example carry the key.

Sign-in returns only to a same-origin path; /connect keeps its query.

Refs #798
…and tests

The image smoke test and OtlpSubprocessExporterTests boot a Production
serving process, which now refuses to start without an https issuer.
Also fixes two mutation-check anchors: the discovery rebase mutant did
not compile, and #797's registration_endpoint anchor moved.

Refs #798
The fixed window is aligned to the clock, so two requests that straddle a
60-second boundary both pass. AuthorizeEndpoint_IsRateLimited failed that
way in CI and TokenEndpoint_IsRateLimited locally. A one-day window, as
#840 did for MultiInstanceRateLimitTests, makes the crossing negligible.

Refs #798
mforce added 2 commits October 9, 2026 06:07
… included

Maintainer decision on review round 1: a session bearer alone must never
mint a code, because the code becomes a token that never expires. A
reconnect still skips the permission explanation; the consent payload now
says alreadyApproved so the SPA can ask for the password only.

Also from round 1:
- The redirect host comes from the redirect URI OpenIddict validated, so a
  single-URI client that omits redirect_uri gets a consent payload rather
  than a 500.
- The expired-session test uses a signed session JWT expired past the clock
  skew; the malformed-token case keeps its own test.
- The sim verifier refuses an issuer with a query or fragment, as the boot
  guard does.

Refs #798
@mforce

mforce commented Oct 9, 2026

Copy link
Copy Markdown
Owner Author

Review round 1, from Codex GPT-6 Astra at head 2624649. Every finding is addressed at dd48441.

P1, the silent skip on a reconnect. The maintainer decided that a reconnect skips the permissions but always asks for the password. Since b0fceb9, no path issues a code without a step-up grant spent through IAccessModule.ConsumeStepUpGrantAsync. The consent payload now carries alreadyApproved, so the SPA can show a password prompt alone for an app the user already approved. docs/decisions/798-oauth-consent.md records the decision and its reason: app tokens never expire, so reconnects are rare and the password costs little. It also records the rejected alternative, which was validating the refresh cookie as a second browser proof.

  • ApprovedApp_AsksOnlyForThePassword: a bearer-only request for an approved app gets no code and gets alreadyApproved: true.
  • Reconnect_SpendsTheGrant: the reconnect spends its grant, and replaying that grant gets 403.
  • Mutants reconnect-without-password (which restores the old skip) and reconnect-grant-unspent are both killed.

P2, an omitted redirect_uri. Fixed in b0fceb9. The redirect host now comes from the URI that OpenIddict validated and stored on ValidateAuthorizationRequestContext, not from Request.RedirectUri. OmittedRedirectUri_UsesTheRegisteredOne sends a request without the parameter for a client with one registered URI, and gets the consent payload with client.example. Mutant redirect-host-unvalidated is killed; it fails with 500 instead of 200.

P3, the expired-session test. Fixed in b0fceb9. ExpiredSession_IsUnauthorized_NotARedirect now sends the user's real session JWT, signed with the test key and expired two minutes ago, which is past the 30-second skew. The malformed case keeps its own test, MalformedBearer_IsUnauthorized_NotARedirect. Mutant session-lifetime-unchecked (ValidateLifetime = false) is killed.

P3, the sim verifier. Fixed in b0fceb9. verify-harness.sh now refuses an issuer with a query or fragment, matching EnsureOAuthIssuer. Run against a freshly bootstrapped .env.sim, it passes https://cluckwork-sim.local/ and fails https://cluckwork-sim.local/?tenant=1.

Verification at dd48441. The OAuth filter and CredentialEpochTests pass locally, and CI is green (18 checks pass; the publish job is skipped, as on every PR). The full tools/oauth/mutation-check.sh run has 83 mutants: 82 are killed and redirect-fragment is held, as before. The baseline and the restore are both green at 100/100. dd48441 only trims two test comments.

@mforce

mforce commented Oct 9, 2026

Copy link
Copy Markdown
Owner Author

Review round 2 by Codex GPT-6 Astra, run locally on head dd48441, found 0 P1, 0 P2 and 2 P3. Every route that produces a code now spends a grant bound to this user and farm, and the redirect crash is fixed.

The two P3s are small, and they will be fixed alongside the UI commits:

  1. OAuthEndpoints.cs:85: the method comment still says a reconnect skips step-up, which contradicts the new unconditional grant check.
  2. tools/simulation/verify-harness.sh:257: the verifier accepts a bare trailing ? or # in the issuer, which the boot guard refuses.

The backend review loop for this PR is complete. The UI waits on the maintainer's consent pick and gets its own review.

mforce added 3 commits October 9, 2026 13:43
…estone

The maintainer approved one raise on 2026-10-09 to cover #800, #798 and
#799. main was at 1,897.12 KiB and #800 measures 1,903.38 KiB; the rest
is reserved for #798 and #799.
The login screen continues to consent and names the app it continues to.
An anonymous authorize request carrying X-Cluckwork-Consent: preview gets
the validated client's self-chosen name and nothing else.

Also from review round 2: the Authorize comment no longer promises a
silent reconnect, and the sim verifier refuses a bare "?" or "#" in
OAuth__Issuer, as the boot guard does.

Refs #798
mforce added 2 commits October 9, 2026 14:00
…ing, and the OAuth screen picks

The write permission reads Record daily entries with no promise about
drafts; #809 adds that once the write tool's behaviour is settled. The
maintainer approved a second milestone precache raise to 1,950 KiB.
docs/designs/788-oauth-screens/ carries the direction lab and the picks.

Refs #798
The password field is required, so an empty submit is refused by the form.

Refs #798
@mforce

mforce commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner Author

Screenshots for #798, captured at 1:1 from the isolated cw798 stack rebuilt at head 46e7ee0. The stack runs Production config behind an https edge (https://localhost:9443) with the service worker active. The account is sim-worker-1, a Worker on the simulation fixture. Each pair is 1280 and 390.

Login, before. An ordinary sign-in. This code path does not change in this PR, and before this PR a sign-in that continued to a connected app looked the same, because /connect did not exist.

login before 1280 login before 390

Login B, after. Sign-in that continues to a connected app names the app. The box sits in the brand panel at 1280 and above the form at 390.

login B 1280 login B 390

Consent D. The account email sits at the top in normal text colour and semibold weight.

consent 1280 consent 390

Wrong password.

wrong password 1280 wrong password 390

Asks for more. The app was allowed to read farm data, and now also asks to record daily entries.

widened 1280 widened 390

Details open. For this flock-scoped Worker, it names the one assigned flock the app could reach.

details 1280 details 390

Reconnect. The app was already approved for everything it asks, so the screen asks for the password only.

reconnect 1280 reconnect 390

#778 proof, on the built SPA with the service worker in control. The same run as the screenshots, recorded by the capture script:

  • The service worker was activated and controlling /login before the hop.
  • A browser navigation to /api/v1/oauth/authorize?... went to /connect, and then to /login, which named the app. The service worker still controlled the page.
  • After sign-in, the browser was back on /connect with the original request intact.
  • A wrong password showed "Wrong password. Nothing was connected."
  • Allow with the password sent the browser to the app's loopback callback, with code and state=cw798.
  • The password-only reconnect issued a code, and the token endpoint redeemed it with 200 and an access_token.
  • Discovery on the edge advertised https://localhost:9443/ endpoints, built from OAuth__Issuer.

mforce added 2 commits October 9, 2026 14:22
CodeQL read the lab's long-strings length estimate as an incomplete
sanitizer. It counts the characters outside markup instead; all 207
frames render identically and without page errors.

Refs #798
@mforce mforce changed the title feat(api): require step-up consent for connected apps and run the OAuth server in Production feat: add the connected-app consent screen with step-up, and run the OAuth server in Production Oct 9, 2026
mforce added 5 commits October 9, 2026 15:01
Resolutions:
- web/scripts/verify-sw.mjs and docs/designs/822-mui-revamp.md: the
  ceiling is 1,950 KiB, with #800's and #798's dated notes both kept.
- OAuthEndpoints.Authorize keeps #798's consent with step-up and adds #800's
  client_name claim, read from the application the handler already loaded.
- AuditConnectedAppTests connects through the step-up approval, since the
  self-approving stub is gone.

Refs #798
…e request

Review round 3 (Codex GPT-6 Astra):
- Details built its access sentence from the role name alone. The consent
  payload now carries assignedFlocks, the flock scope this request resolved
  (null for every flock, else the assigned flocks' names, through
  IAccessModule), and Details states what the requested scopes would get
  within that scope and the role.
- Focus starts at the headline instead of the password, for consent and
  reconnect; a wrong password returns focus to the field.
- The account email drops body1's tabular figures, which spaced its hyphens.

Refs #798
@mforce

mforce commented Oct 9, 2026

Copy link
Copy Markdown
Owner Author

Review round 3, from Codex GPT-6 Astra at head 6368134, and the merge with main. Everything below is at 887a7b2.

Merge with #800. f044a04 merges origin/main (9934ef7) as a merge commit.

P2, Details described access from the role alone. Fixed in d4cc60d.

  • The consent payload now carries assignedFlocks. It is the flock scope the request itself resolved: null for every flock, which includes a Worker with no assignments, else the assigned flocks' names, read through IAccessModule.ListFlockAssignmentsAsync.
  • Details now states what the requested scopes would get, within that flock scope and the role. A read-only request makes no promise about recording.
  • Server tests: Payload_NamesTheAssignedFlocks_OfAScopedWorker and Payload_SaysEveryFlock_ForAnUnassignedWorker. The mutants scoped-worker-unrestricted and unassigned-worker-scoped are both killed.
  • Vitest specs: a Worker with assignments, a Worker with none, a Manager asking only to read, and a Manager asking for both. Ignoring assignedFlocks fails the first, and choosing the write sentence by role alone fails the third.

P2, password autofocus skipped the request. Fixed in d4cc60d.

  • The headline (tabIndex={-1}) takes focus when the consent card or the reconnect card loads. A wrong password returns focus to the field.
  • Specs cover initial focus for consent and for reconnect, and focus after a wrong password. Removing the heading focus fails both initial-focus specs, and removing the retry focus fails the third.
  • Putting autoFocus back on the password survives. It is an equivalent mutant, because the heading-focus effect runs after the input mounts and takes focus back.

P3, the email's hyphens. Fixed in d4cc60d. The email sets fontVariantNumeric: "normal" over body1's tabular figures.

Verification.

  • ConnectPage passes 18/18, and the wider web run (i18n, Help, glossary, styles, Login, AuthShell, Users) passes 886/886.
  • I reran all 27 mutants on OAuthEndpoints.cs after the merge, and all 27 are killed. One needed its anchor regenerated first (887a7b2), because the payload gained a field.
  • cw798 was rebuilt at 46e7ee0. The Service worker swallows server-issued redirects, so ?farm= never reaches the login form #778 flow passed again: the service worker controlled the hop, the request survived sign-in, the wrong-password message appeared, and the reconnect code redeemed with 200.
  • The screenshot comment above now shows all 14 frames from that build, because the focus change touches every consent frame. Details open names this Worker's assigned flock, and the email reads normally. Each image returns 200. The stack is torn down and its images removed.

@mforce

mforce commented Oct 9, 2026

Copy link
Copy Markdown
Owner Author

UI round 2 by Codex GPT-6 Astra, run locally on head 887a7b2, found 0 P1, 0 P2 and 0 P3. It confirmed:

  • assignedFlocks is built for the authenticated user through IAccessModule, null exactly when the scope is unrestricted, and absent from the anonymous preview.
  • The Details sentence matches real access for each role, assignment and scope combination.
  • Focus starts on the headline and moves to the password after a wrong one.
  • The merge keeps the step-up consent and OAuth slice 6: audit provenance — record which application acted, alongside the human #800's client_name claim. AuditConnectedAppTests was updated faithfully, not weakened.

The review loop for this PR is complete. Release note: this PR ships in the same release as #799, and the deployment repo must set OAuth__Issuer before that release is deployed.

@mforce
mforce merged commit e40924b into main Oct 9, 2026
19 checks passed
@mforce
mforce deleted the feat/798-oauth-consent branch October 9, 2026 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth slice 4: consent screen, with step-up and return-URL validation

2 participants